Refactor file utilities into dedicated package and improve plugin validation #211
Conversation
* Improve plugin config validation on-load
WalkthroughAdds a new internal/files package implementing XDG-compliant user paths, directory creation/permission checks, and executable discovery. Multiple packages delegate directory and executable discovery responsibilities to it; plugin validation now checks configured plugin names exist in a specified plugin directory. Changes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/files/files.go (1)
16-16: Minor typo in comment."cache file" should be "cache files" for consistency with line 13.
Apply this diff:
- // EnvVarXDGCacheHome is the XDG Base Directory env var name for cache file. + // EnvVarXDGCacheHome is the XDG Base Directory env var name for cache files.internal/files/files_test.go (3)
288-295: Consider using a more restrictive permission or renaming the test case.The test case is named "accepts directory with more restrictive permissions", but it creates the directory with
0o700, which is identical toperms.SecureDir(0o700). This duplicates the "exact required permissions" test case at lines 279-286 rather than testing a more restrictive scenario.To improve clarity, either rename the test case to reflect that it's another exact match scenario, or use a more restrictive permission such as
0o600or0o000:{ - name: "accepts directory with more restrictive permissions", + name: "accepts directory with 0o600 (more restrictive)", setup: func(t *testing.T) string { dir := filepath.Join(t.TempDir(), "more-restrictive") - require.NoError(t, os.Mkdir(dir, 0o700)) + require.NoError(t, os.Mkdir(dir, 0o600)) return dir }, wantErr: false, },
395-397: Simplify directory creation.The test creates a directory with
0o755, then immediately changes it to0o777withos.Chmod. Since only the final0o777state is needed for testing, you can simplify by creating directly with0o777:setup: func(t *testing.T) string { dir := filepath.Join(t.TempDir(), "less-restrictive") - require.NoError(t, os.Mkdir(dir, 0o755)) - require.NoError(t, os.Chmod(dir, 0o777)) + require.NoError(t, os.Mkdir(dir, 0o777)) return dir },
444-445: Simplify directory creation.Similar to the test case at lines 395-397, this creates a directory with
0o755, then immediately changes it to0o777. This can be simplified:tempDir := t.TempDir() tooOpen := filepath.Join(tempDir, "too-open") -require.NoError(t, os.Mkdir(tooOpen, 0o755)) -require.NoError(t, os.Chmod(tooOpen, 0o777)) +require.NoError(t, os.Mkdir(tooOpen, 0o777))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/cache/cache.go(2 hunks)internal/config/plugin_config.go(2 hunks)internal/context/context.go(2 hunks)internal/context/context_test.go(0 hunks)internal/files/files.go(1 hunks)internal/files/files_test.go(1 hunks)internal/flags/config.go(2 hunks)internal/flags/config_test.go(6 hunks)internal/plugin/manager.go(3 hunks)internal/registry/options/build_options.go(2 hunks)
💤 Files with no reviewable changes (1)
- internal/context/context_test.go
🧰 Additional context used
🧬 Code graph analysis (9)
internal/registry/options/build_options.go (1)
internal/files/files.go (1)
UserSpecificCacheDir(118-120)
internal/cache/cache.go (1)
internal/files/files.go (1)
EnsureAtLeastRegularDir(102-104)
internal/flags/config_test.go (2)
internal/files/files.go (2)
UserSpecificConfigDir(126-128)EnvVarXDGConfigHome(14-14)internal/flags/config.go (1)
RuntimeFile(54-54)
internal/flags/config.go (1)
internal/files/files.go (1)
UserSpecificConfigDir(126-128)
internal/config/plugin_config.go (1)
internal/files/files.go (1)
DiscoverExecutables(27-55)
internal/files/files.go (1)
internal/perms/perms.go (2)
RegularDir(22-22)SecureDir(26-26)
internal/plugin/manager.go (1)
internal/files/files.go (1)
DiscoverExecutablesWithPaths(60-96)
internal/files/files_test.go (2)
internal/files/files.go (9)
AppDirName(21-23)EnvVarXDGConfigHome(14-14)UserSpecificConfigDir(126-128)EnvVarXDGCacheHome(17-17)UserSpecificCacheDir(118-120)EnsureAtLeastSecureDir(110-112)EnsureAtLeastRegularDir(102-104)DiscoverExecutables(27-55)DiscoverExecutablesWithPaths(60-96)internal/perms/perms.go (2)
SecureDir(26-26)RegularDir(22-22)
internal/context/context.go (2)
internal/files/files.go (2)
EnsureAtLeastSecureDir(110-112)EnsureAtLeastRegularDir(102-104)internal/perms/perms.go (2)
SecureFile(15-15)RegularFile(11-11)
🔇 Additional comments (25)
internal/cache/cache.go (2)
14-14: LGTM! Import refactored to use centralized files package.The import change aligns with the PR objective of consolidating file utilities into
internal/files.
45-45: LGTM! Function call updated to use the new files package.The behaviour remains identical as confirmed by the relevant code snippets.
internal/files/files.go (8)
20-23: LGTM! Clean implementation.The function correctly returns the application directory name.
25-55: LGTM! Robust executable discovery implementation.The function correctly:
- Filters directories and hidden files
- Checks execute permissions using the appropriate bit mask
- Returns errors appropriately
57-96: LGTM! Well-implemented discovery with filtering.The function correctly handles optional allowlist filtering and returns full paths for discovered executables.
98-112: LGTM! Clean wrapper functions with clear documentation.The functions correctly delegate to the internal helper and clearly document that they do not attempt to repair permissions.
114-128: LGTM! XDG-compliant directory resolution.Both functions correctly implement the XDG Base Directory Specification with clear documentation.
130-152: LGTM! Robust directory creation with permission validation.The function correctly creates directories and validates permissions without attempting unsafe repairs.
154-160: LGTM! Correct permission validation logic.The bitwise operation correctly verifies that actual permissions do not grant any permissions beyond those in required.
162-188: LGTM! Well-structured XDG directory resolution.The function correctly:
- Validates XDG naming conventions
- Handles environment variable precedence
- Falls back to standard home directory paths
- Provides clear error messages
internal/config/plugin_config.go (3)
9-9: LGTM! Import added for new validation functionality.The import is required for the plugin directory validation added below.
251-254: LGTM! Plugin directory validation integrated appropriately.The validation is correctly placed after individual plugin checks and follows the established error collection pattern.
259-288: LGTM! Comprehensive plugin validation implementation.The validation correctly:
- Skips validation when plugin directory is not configured
- Discovers executables in the configured directory
- Verifies all configured plugins exist as executables
- Provides descriptive error messages including file paths
internal/registry/options/build_options.go (2)
8-8: LGTM! Import refactored to use centralized files package.Consistent with the PR's consolidation of file utilities.
86-86: LGTM! Function call updated to use the new files package.The behaviour remains identical per the relevant code snippets.
internal/flags/config.go (2)
11-11: LGTM! Import refactored to use centralized files package.Consistent with the PR's consolidation of file utilities.
83-83: LGTM! Function call updated to use the new files package.The behaviour remains identical per the relevant code snippets.
internal/plugin/manager.go (3)
28-28: LGTM! Import added for executable discovery utilities.Required for the refactored plugin discovery at line 335.
335-338: LGTM! Plugin discovery refactored to use centralized utilities.The change delegates discovery logic to
files.DiscoverExecutablesWithPaths, which is consistent with the PR's consolidation objectives and simplifies this code.
383-385: Verify the removal ofInferLevelsWithTimestamp.The removal of
InferLevelsWithTimestampfrom the logger options appears unrelated to the file utilities refactoring. Ensure this change is intentional and does not negatively impact log formatting for plugin output.Could you confirm whether this logging change was intentional and whether it might affect debugging or observability?
internal/flags/config_test.go (1)
11-11: LGTM! Test file refactored to use centralized files package.All changes consistently update imports, function calls, and environment variable references to use the new
internal/filespackage, ensuring tests remain aligned with the refactored implementation.Also applies to: 253-253, 263-263, 272-272, 278-278, 281-281, 345-345, 354-354, 357-357
internal/context/context.go (3)
15-15: LGTM! Import added for centralized file utilities.Required for the refactored directory operations below.
122-122: LGTM! SaveConfig refactored to use centralized utilities.The function now delegates secure directory creation to
files.EnsureAtLeastSecureDir, which is consistent with the PR's consolidation objectives.
128-128: LGTM! SaveExportedConfig refactored to use centralized utilities.The function now delegates regular directory creation to
files.EnsureAtLeastRegularDir, which is consistent with the PR's consolidation objectives.internal/files/files_test.go (1)
453-600: Excellent test coverage for discovery functions.The tests comprehensively cover all scenarios for
DiscoverExecutablesandDiscoverExecutablesWithPaths, including edge cases such as non-existent directories, empty directories, filtering logic, hidden files, and subdirectories. The test assertions are clear and appropriate.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @peteski22. * #211 (comment) The following files were modified: * `internal/cache/cache.go` * `internal/context/context.go` * `internal/files/files.go` * `internal/flags/config.go` * `internal/registry/options/build_options.go`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/files/files.go (1)
25-96: Consider consolidating the discover functions to reduce duplication.The
DiscoverExecutablesandDiscoverExecutablesWithPathsfunctions share substantial logic for scanning directories, filtering by type, checking hidden files, and validating execute permissions. You could refactorDiscoverExecutablesto callDiscoverExecutablesWithPaths(dir, nil)and transform the result, or extract the common scanning logic into a helper function.Example refactor:
func DiscoverExecutables(dir string) (map[string]struct{}, error) { - entries, err := os.ReadDir(dir) + executables, err := DiscoverExecutablesWithPaths(dir, nil) if err != nil { return nil, err } - executables := make(map[string]struct{}) - for _, entry := range entries { - if entry.IsDir() { - continue - } - - if strings.HasPrefix(entry.Name(), ".") { - continue - } - - info, err := entry.Info() - if err != nil { - return nil, fmt.Errorf("reading file info for %s: %w", entry.Name(), err) - } - - // Check for execute permission (0o111 = user/group/other execute bits). - if info.Mode()&0o111 != 0 { - executables[entry.Name()] = struct{}{} - } + result := make(map[string]struct{}, len(executables)) + for name := range executables { + result[name] = struct{}{} } - - return executables, nil + return result, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/files/files.go(1 hunks)internal/files/files_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/files/files_test.go (2)
internal/files/files.go (9)
AppDirName(21-23)EnvVarXDGConfigHome(14-14)UserSpecificConfigDir(126-128)EnvVarXDGCacheHome(17-17)UserSpecificCacheDir(118-120)EnsureAtLeastSecureDir(110-112)EnsureAtLeastRegularDir(102-104)DiscoverExecutables(27-55)DiscoverExecutablesWithPaths(60-96)internal/perms/perms.go (2)
SecureDir(26-26)RegularDir(22-22)
internal/files/files.go (1)
internal/perms/perms.go (2)
RegularDir(22-22)SecureDir(26-26)
🔇 Additional comments (9)
internal/files/files_test.go (5)
14-120: Excellent test coverage for directory resolution.The tests for
AppDirName,UserSpecificConfigDir, andUserSpecificCacheDirare comprehensive and well-structured. They properly cover XDG environment variable handling, whitespace trimming, and fallback behaviour. The use oft.Setenvensures proper test isolation.
158-260: Thorough permission validation tests.The permission acceptance tests are comprehensive, covering exact matches, more restrictive scenarios, less restrictive scenarios, and various permission patterns. The test naming clearly documents expected behaviour.
262-449: Solid directory creation and validation tests.The tests for
EnsureAtLeastSecureDirandEnsureAtLeastRegularDirproperly validate directory creation, permission enforcement, nested path handling, and error messaging. The setup functions provide clear test isolation.
451-550: Comprehensive executable discovery tests.The tests for
DiscoverExecutablescover all filtering conditions—non-existent directories, empty directories, executable vs. non-executable files, hidden files, and subdirectories. The tests use actual file creation with appropriate permissions to validate behaviour.
552-598: Good coverage for path-based discovery with filtering.The tests for
DiscoverExecutablesWithPathsproperly validate the allowlist filtering behaviour and the nil allowlist case. The verification of full paths ensures correct path construction.internal/files/files.go (4)
12-23: Clean constant definitions and application name.The XDG environment variable constants and
AppDirNamefunction are straightforward and correctly implemented.
98-152: Well-implemented directory creation and validation.The
EnsureAtLeastRegularDir,EnsureAtLeastSecureDir, and underlyingensureAtLeastDirfunction correctly handle directory creation withMkdirAlland validate permissions without attempting to repair them. Error messages clearly indicate the actual vs. required permissions.
154-188: Correct permission logic and XDG-compliant directory resolution.The
isPermissionAcceptablefunction correctly validates that actual permissions don't grant any rights beyond the required permissions using bitwise operations. TheuserSpecificDirfunction properly implements XDG Base Directory Specification with environment variable validation, whitespace handling, and appropriate fallbacks.
114-128: Clear XDG directory resolution functions.The
UserSpecificCacheDirandUserSpecificConfigDirfunctions are well-documented wrappers that correctly delegate to the underlying XDG-compliant implementation. The comments appropriately reference the specification.
peteski22
force-pushed
the
peteski22/refactor-file-helpers
branch
from
434f970 to
f5af7db
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/files/files.go (2)
25-55: Windows support and case-insensitive allow-list (optional).On Windows, exec bits are meaningless; executables are recognised by extension (PATHEXT). Also the filesystem is case-insensitive, so allowed lookups should normalise. Consider a small helper and platform-specific checks.
Example helper (add once, then use in both functions):
// isExecutableByPlatform reports whether path is executable for the current OS. func isExecutableByPlatform(path string, fi os.FileInfo) bool { if fi.IsDir() { return false } if runtime.GOOS == "windows" { ext := strings.ToLower(filepath.Ext(path)) pathext := os.Getenv("PATHEXT") if pathext == "" { pathext = ".COM;.EXE;.BAT;.CMD" } for _, e := range strings.Split(strings.ToLower(pathext), ";") { if ext == strings.TrimSpace(e) { return true } } return false } return fi.Mode().Perm()&0o111 != 0 }If you adopt this, also normalise the allowed set on Windows using strings.EqualFold or a lower-cased mirror map.
Also applies to: 57-96
116-118: Docs nit: trailing slash in comments.The functions return paths without a trailing slash; update comments to avoid the trailing “/”.
Also applies to: 124-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/files/files.go(1 hunks)internal/files/files_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/files/files_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/files/files.go (1)
internal/perms/perms.go (2)
RegularDir(22-22)SecureDir(26-26)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/files/files.go (3)
67-76: Follow symlinks and only accept regular executable files; make scan resilient.DirEntry.Info() lstats the entry; symlinks usually appear as 0777, so broken/non-exec targets are misclassified as executables. Also, a single per-entry error currently aborts the whole scan. Use os.Stat on the full path (follow symlinks), require a regular file, and skip unreadable/broken entries.
Apply this diff:
- info, err := entry.Info() - if err != nil { - return nil, fmt.Errorf("reading file info for %s: %w", entry.Name(), err) - } - - // Check for execute permission (0o111 = user/group/other execute bits). - if info.Mode()&0o111 != 0 { - fullPath := filepath.Join(dir, entry.Name()) - executables[entry.Name()] = fullPath - } + fullPath := filepath.Join(dir, entry.Name()) + info, err := os.Stat(fullPath) // follow symlinks + if err != nil { + // Skip broken symlinks or unreadable entries + continue + } + // Only regular files with any execute bit set + if info.Mode().IsRegular() && (info.Mode().Perm()&0o111 != 0) { + executables[entry.Name()] = fullPath + }
122-133: Reject symlinked or non-directory paths for secure/regular dirs.For "secure" usage, accepting a symlink can defeat the permission intent. Validate with Lstat, forbid symlinks, and ensure the path is a directory before checking perms.
- info, err := os.Stat(path) + info, err := os.Lstat(path) if err != nil { - return fmt.Errorf("could not stat directory '%s': %w", path, err) + return fmt.Errorf("could not lstat directory '%s': %w", path, err) } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("directory '%s' must not be a symlink", path) + } + if !info.IsDir() { + return fmt.Errorf("path '%s' exists but is not a directory", path) + } if !isPermissionAcceptable(info.Mode().Perm(), perm) { return fmt.Errorf( "incorrect permissions for directory '%s' (%#o, want %#o or more restrictive)", path, info.Mode().Perm(), perm, ) }
160-162: Enforce absolute XDG paths per spec.XDG_*_HOME must be an absolute path; reject relative values to avoid surprising working-directory relative writes.
- if ch, ok := os.LookupEnv(envVar); ok && strings.TrimSpace(ch) != "" { - home := strings.TrimSpace(ch) - return filepath.Join(home, AppDirName()), nil - } + if ch, ok := os.LookupEnv(envVar); ok && strings.TrimSpace(ch) != "" { + base := strings.TrimSpace(ch) + if !filepath.IsAbs(base) { + return "", fmt.Errorf("%s must be an absolute path (XDG spec), got %q", envVar, base) + } + return filepath.Join(base, AppDirName()), nil + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/files/files.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/files/files.go (1)
internal/perms/perms.go (2)
RegularDir(22-22)SecureDir(26-26)
🔇 Additional comments (1)
internal/files/files.go (1)
138-144: LGTM: Permission check logic is correct.The bitwise logic correctly verifies that actual permissions are equal to or more restrictive than required permissions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/files/files.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/files/files.go (1)
internal/perms/perms.go (2)
RegularDir(22-22)SecureDir(26-26)
🔇 Additional comments (10)
internal/files/files.go (10)
1-10: LGTM! Clean package structure.The package declaration and imports are well-organised and include only necessary dependencies.
12-18: LGTM! Clear XDG constant definitions.The XDG environment variable constants are well-documented and follow naming conventions.
20-23: LGTM! Simple and clear.The function provides a consistent application directory name with clear documentation.
25-39: LGTM! Clean delegation pattern.The function properly delegates to
DiscoverExecutablesWithPathsand efficiently converts the result to a set representation.
41-83: LGTM! Previous concerns properly addressed.The function now correctly:
- Follows symlinks using
os.Stat(line 70)- Validates regular files with
IsRegular()(line 77)- Gracefully skips broken symlinks or unreadable entries (lines 71-74)
- Checks for execute permissions appropriately (line 77)
The implementation is resilient and handles edge cases well.
85-99: LGTM! Clean permission enforcement pattern.Both functions appropriately delegate to
ensureAtLeastDirwith the correct permission constants and clearly document that they do not repair existing permissions.
101-115: LGTM! XDG-compliant path resolution.Both functions properly delegate to
userSpecificDirwith appropriate XDG environment variables and fallback directories, with excellent documentation referencing the XDG specification.
117-153: LGTM! Security concerns properly addressed.The function now correctly:
- Uses
os.Lstatto avoid following symlinks (line 131)- Explicitly rejects symlinks for security (lines 136-138)
- Validates the path is a directory (lines 140-142)
- Documents the
os.MkdirAllintermediate directory behaviour with a clear NOTE (lines 122-125)The implementation provides appropriate security guarantees for the intended use case.
155-161: LGTM! Correct permission validation logic.The bit-mask logic correctly determines whether actual permissions are equal to or more restrictive than required permissions. The documentation clearly explains the intent.
1-193: Excellent work addressing all previous review concerns!This file demonstrates high code quality with comprehensive security considerations:
✅ Symlink following and regular file validation in executable discovery
✅ Symlink rejection in directory validation
✅ XDG absolute path enforcement
✅ Clear documentation ofos.MkdirAllbehaviourThe implementation is robust, well-documented, and follows Go best practices. All major issues from previous reviews have been properly resolved.
2 participants
Summary
Creates
internal/filespackage to consolidate file and directory utilities, and adds validation to ensure configured plugins exist at config load time.Changes
internal/fileswith utilities extracted frominternal/context:UserSpecificConfigDir(),UserSpecificCacheDir()EnsureAtLeastSecureDir(),EnsureAtLeastRegularDir()DiscoverExecutables(),DiscoverExecutablesWithPaths()AppDirName()PluginConfig.Validate():Summary by CodeRabbit
Bug Fixes
Refactor
Behaviour
Tests